feat(pusher): harden native RollingUpdate (SCA-39 long-term rollout, DRAFT)#10280
feat(pusher): harden native RollingUpdate (SCA-39 long-term rollout, DRAFT)#10280Git-on-my-level wants to merge 6 commits into
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c5dc3f5562
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c5dc3f5562
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
c5dc3f5 to
4dfdb5b
Compare
c4f44b6 to
73a6fc7
Compare
Monitor: review-feedback fixes pushed (73a6fc7)Addressed all three Codex P2 review threads in one commit.
New regression tests:
Verification: all All three threads resolved. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 73a6fc75d7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
92cdfd4 to
bcdcef0
Compare
Monitor: wired shared-config migration guard into CI (bcdcef0)Addressed the remaining Codex P2 review thread.
Regression tests: 5 new tests covering preflight pass on real chart values, default-mode behavior, malformed-binding detection, guard-mode argument validation, and explicit guard-mode compatibility. All 18 tests in CI status: Hygiene, Formatting, Desktop Swift (all 3 checks), and backend preflight checks now pass. The remaining Thread resolved. |
SCA-39 long-term rollout strategy (DRAFT, architecture review only): - split /ready (readiness) from /health (liveness) + loopback-only drain endpoint + reject new WS during drain (in-flight sessions keep draining) - chart readinessProbe->/ready, BackendConfig connectionDraining (60s <= grace), preStop flips readiness before the NEG-convergence sleep - fail-closed rollout preflight + read-only rollback contract; fail-closed on missing rollout-blocking telemetry - operator/dev runbook + N/N-1 compatibility checklist + dev qualification plan Defers build-once digest promotion to SCA-40 (owns digest/chart-only mode/ REDIS_DB_HOST preflight/rollback evidence); no deploy, no mutation, no CRDs.
Absorbs the durable long-term pieces from the superseded incident recovery (#10278) into the hardened RollingUpdate contract — no Argo/Flagger/mesh/Gateway. Exact-digest build-once promotion contract (incident-class artifact fix): - chart renders repository@sha256:<hex> when image.digest is set; rejects malformed/ambiguous identity, and a digest + mutable tag, at render time - rollout gate admits a digest only with tag dropped and pullPolicy IfNotPresent (the digest is the content address; Always only adds a registry round-trip); tag mode (chart delegates the tag to the deploy workflow) is unchanged - rollback contract restore is digest-aware; stale SCA-40 ownership notes removed Cross-resource shared-config migration guard (2026-07-22 incident root fix): - new verify_shared_config_migration.py fails CLOSED before any mutation when a serving workload still references a ConfigMap/Secret source or key the proposed state removes or reclassifies — the exact partial transition that dropped prod Pusher to zero healthy pods while /health stayed green - reads object/key NAMES only; never reads, prints, or stores values - transition-aware: a key moving between objects is allowed only once no stale old-source reference remains Discards the incident-only chart-only recovery CLI surface and its broad normalize() (which could hide real chart policy drift), per SCA-39 objective. Coverage: 4 new gate digest tests + 9 migration-guard fixtures incl. the historical partial transition and fail-closed drift. End-to-end verified against the real rendered dev Pusher chart (22 refs resolve; a depleted REDIS_DB_HOST inventory is caught).
…ce digest pullPolicy Independent review (reviewer + adversarial refuter) converged on one MAJOR finding: the shared-config migration guard scanned only explicit env.valueFrom refs and was blind to envFrom whole-object bulk loads. Pusher bulk-loads ALL ConfigMap keys via envFrom, so a key removed from that ConfigMap (the exact 2026-07-22 outage class) went undetected when it lacked an explicit ref. - verify_shared_config_migration.py now scans envFrom sources: a bulk-loaded object must still exist, and (given a previous inventory) no key may be removed from it. - Generalizes to every pod-template workload (Deployment/StatefulSet/DaemonSet/Job/CronJob/ReplicaSet) and scans initContainers. - Chart render-enforces image.pullPolicy=IfNotPresent for a digest-pinned release (defense-in-depth matching the gate). Coverage: +4 migration-guard tests (envFrom object/key removal, envFrom clean pass, StatefulSet+initContainer). 81 affected tests pass; real rendered dev Pusher now reports 22 per-key refs + 1 envFrom source.
…I, fix rollback capture/restore Address three Codex review threads (all P2): 1. backend/routers/pusher.py — Move websocket.accept() BEFORE the drain check. A pre-accept close is surfaced as a failed HTTP upgrade by the websockets client, which backend-listen records as a circuit-breaker failure (backend/utils/pusher.py:153-159). During a normal rollout, enough straggler connections can trip the pod's pusher circuit even though other pusher pods are healthy. Accepting first ensures the client sees a clean 1001 Going Away close frame instead. 2. .github/checks-manifest.yaml — Register verify_pusher_rollout_gate.py as 'pusher-rollout-quality-gate' so it actually runs in the pre-push/CI lane. Its docstring claimed to run in the shared pre-push lane but it was never invoked from the check manifest, leaving the fail-closed telemetry/probe/capacity checks as an on-demand-only script. 3. backend/scripts/verify_pusher_rollout_gate.py — Fix the rollback contract capture/restore mismatch. capture_command returned the full image ref (gcr.io/.../pusher:tag), but restore_command substituted it into --set image.tag=<prior-tag>, which would produce an invalid image like gcr.io/.../pusher:gcr.io/.../pusher:tag. Now capture extracts only the tag (sed) or digest (grep -oE), and restore uses --set-string to match the deploy workflows' Helm convention. Verification: - python3 backend/scripts/verify_pusher_rollout_gate.py preflight → OK - python3 backend/scripts/verify_pusher_rollout_gate.py rollback --env prod → valid JSON - ENCRYPTION_SECRET=*** .venv/bin/python -m pytest tests/unit/test_verify_pusher_rollout_gate.py tests/unit/test_pusher_readiness_drain.py → 18 passed
Address Codex P2 thread #3634765197: the guard's docstring claimed it ran in the pre-push lane but was never registered in the checks manifest. - Add self-contained 'preflight' mode (default) that scans pusher chart env/envFrom references directly — same pattern as verify_pusher_rollout_gate. - Register as 'shared-config-migration-preflight' in .github/checks-manifest.yaml with local+ci lanes. - Update existing guard-mode tests to pass explicit 'guard' mode argument. - Add 5 new regression tests covering preflight pass/default/detect/guard-require and explicit guard-mode compatibility.
bcdcef0 to
59d3e8e
Compare
Monitor: unblocked backend unit suite after rebase (59d3e8e)Fixed the failing backend unit check after the monitor-rebased PR picked up the current desktop signed-artifact smoke contract.
Verification:
|
SCA-39 — Harden pusher native RollingUpdate (long-term rollout strategy)
DRAFT — architecture review only. Implements the accepted PRIMARY from the
SCA-39 Phase-0 decision: harden the existing native Kubernetes RollingUpdate
for the pusher GKE workload. No Argo / Flagger / service mesh / Gateway API, and
no two-color (A2) experiment — those remain documented variants only.
Dependency / branch
origin/main(241f1c4c4dc6). Not stacked on SCA-40 because SCA-40(
c0590c68) was still in_progress with no published branch at start; this PRimplements only the non-overlapping long-term pieces and must be rebased
onto SCA-40's head if/when it lands (digest promotion / chart-only mode /
REDIS_DB_HOST preflight / rollback evidence are SCA-40's and are not
duplicated here). The quality-gate script explicitly asserts
image.digestis absent and will need a lockstep update alongside SCA-40's digest field.
Changed surfaces
Runtime (Lane A)
backend/utils/readiness.py(new): reusable, dependency-freeReadinessGate(SERVING/DRAINING), thread-safe, idempotent
begin_drain(); designed forpusher (wired) and the llm-gateway (planned).
backend/pusher/main.py:/health(liveness, 200 while alive), new/ready(503 when draining), new loopback-only
POST /__internal/drain; lifespanshutdown calls
begin_drain()first then drains background tasks.backend/routers/pusher.py: rejects new WS (close 1001 before accept)during drain; in-flight sessions keep draining via their own bounded logic.
backend/utils/metrics.py: two low-cardinality gaugespusher_ready,pusher_drain_in_progress.Helm/GKE contract (Lane B)
backend/charts/pusher/{dev,prod}_omi_pusher_values.yaml:readinessProbe→/ready(liveness/startup stay/health);preStopnowcurl -sf -m 5 -X POST localhost:8080/__internal/drain || true; sleep 15(flip readiness early + NEG-convergence sleep); added
backendConfig.connectionDraining.drainingTimeoutSec: 60.backend/charts/pusher/templates/backendconfig.yaml: addedconnectionDraining.drainingTimeoutSec; healthCheck stays on/health(liveness semantics — routing it to
/readywould defeat connectionDraining).Quality gates + rollback contract (Lane D)
backend/scripts/verify_pusher_rollout_gate.py(new): fail-closedpreflight(capacity headroom, image-identity, probe split, telemetry-present) + a
read-only
rollbackcontract (emits JSON; never executes). Missing telemetry⇒ fail, not green.
backend/scripts/verify_pusher_rollout_budget.py: extended to also enforce theprobe split, connectionDraining,
drainingTimeoutSec ≤ grace, and theBackendConfig healthCheck invariant (the highest-value guard added from the
refuter pass).
Docs (Lane E)
backend/docs/pusher_rolling_update_operations.md(new): honest availabilitycontract, termination sequence, operator runbook, fail-closed gates, N/N-1
compatibility checklist, dev qualification plan, SCA-40 relationship.
Honest availability contract (no overclaim)
In-flight WebSocket sessions are not migrated across a cutover over the
proxied ALB+NEG path. "Graceful drain" = zero new-connection rejection + a
bounded ~1–60s reconnect gap per affected session (backend-listen reconnects);
connectionDrainingbounds how long the LB waits before cutting. No sub-secondor zero-session-impact cutover is claimed.
Tests
test_pusher_readiness_drain.py(6): serving/drain, loopback-only, idempotent,non-loopback 403, reset, real-route wiring + shutdown-drain.
test_verify_pusher_rollout_budget.py(+6) andtest_verify_pusher_rollout_gate.py(6):good-chart passes; fail-closed regressions for probe routing, missing
connectionDraining/healthCheck invariant, drain>grace, missing telemetry.
make setup;helm templatedev+prod (liveness/health,readiness
/ready, BackendConfig healthCheck/health,connectionDraining60 ≤ grace 120); 81 affected unit tests pass with the worktree venv;
scripts/pr-preflight --suggestclean.Known gaps / remaining
draft; the automated check is a static definition-presence scan only (tracked).
deferred (wiring it could itself rebuild); both scripts are runnable standalone.
Boundary (DRAFT)
No dev/prod deployment, no controller/CRD install, no production mutation, no
workflow dispatch. Production adoption is blocked on a healthy Pusher
baseline (SCA-36/SCA-40 repair) plus explicit approval — this PR is for
review only.